refactor(store): get account vault details from account state forest#1981
refactor(store): get account vault details from account state forest#1981
Conversation
GetAccount.details.vault_details now comes from AccountStateForest instead of select_account_vault_at_block(). I added a historical vault enumeration API to AccountStateForest and switched the call site in State::fetch_public_account_details() to use it. The rest of the details path is unchanged: header, storage header, code lookup, and storage map assembly still use the existing SQLite logic.
kkovaacs
requested review from
Mirko-von-Leipzig,
bobbinth,
Copilot and
sergerad
There was a problem hiding this comment.
Pull request overview
Refactors the store’s GetAccount path to reconstruct account vault assets from the in-memory AccountStateForest instead of querying SQLite, aligning with #1978’s performance goals by reducing DB round-trips.
Changes:
- Switch
State::fetch_public_account_details()vault-details lookup fromselect_account_vault_at_block()toAccountStateForest::get_vault_details(). - Add
AccountStateForest::get_vault_details(account_id, block_num)API to enumerate historical vault contents. - Add unit tests for latest/historical vault enumeration and
MAX_RETURN_ENTRIESlimit handling; update changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/store/src/state/mod.rs | Uses AccountStateForest for vault_details reconstruction (and updates tracing spans). |
| crates/store/src/account_state_forest/mod.rs | Adds get_vault_details() vault enumeration at a given block. |
| crates/store/src/account_state_forest/tests.rs | Adds tests covering historical/latest vault assets and limit-exceeded behavior. |
| CHANGELOG.md | Notes the GetAccount vault-assets optimization for the next release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
LGTM, maybe we do want that pre-check that co-pilot suggested?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map_err(|err| { | ||
| DatabaseError::DataCorrupted(format!( | ||
| "failed to reconstruct vault for account {account_id} at block {block_num}: {err}" | ||
| )) |
There was a problem hiding this comment.
get_vault_details() errors are all being flattened into DatabaseError::DataCorrupted(...), which loses the original error kind (e.g. Merkle vs asset parsing vs missing root) and makes troubleshooting harder. Consider matching on WitnessError and mapping each variant to the closest DatabaseError variant (e.g. MerkleError/AssetError/StorageMapError), using DataCorrupted only for the truly invariant-violating cases (like RootNotFound).
| .map_err(|err| { | |
| DatabaseError::DataCorrupted(format!( | |
| "failed to reconstruct vault for account {account_id} at block {block_num}: {err}" | |
| )) | |
| .map_err(|err| match err { | |
| WitnessError::MerkleError(err) => DatabaseError::MerkleError(err), | |
| WitnessError::AssetError(err) => DatabaseError::AssetError(err), | |
| WitnessError::StorageMapError(err) => DatabaseError::StorageMapError(err), | |
| WitnessError::RootNotFound(root) => DatabaseError::DataCorrupted(format!( | |
| "failed to reconstruct vault for account {account_id} at block {block_num}: missing root {root}" | |
| )), |
|
|
||
| ## v0.14.10 (TBD) | ||
|
|
||
| - Optimize `GetAccount` implementation to serve vault assets from `AccountStateForest`. |
There was a problem hiding this comment.
nit: let's add a link to the PR here.
| .map(|entry| Asset::from_key_value_words(entry.key, entry.value)) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
|
|
||
| Ok(AccountVaultDetails::from_assets(assets)) |
There was a problem hiding this comment.
This is probably fine for now, but in the future, I'd move the logic for figuring out whether we should return the list of assets or LimitExceeded from AccountVaultDetails::from_assets() to here.
As outlined in #1978 we can avoid some DB queries since we have most account details in
AccountStateForest. We were already doing that for specific storage map key requests, but account vault assets were still queried from the database even though we do have those inAccountStateForestas well.This PR implements the
vault_detailslookup to useAccountStateForestinstead ofselect_account_vault_at_block(). I added a historical vault enumeration API toAccountStateForestand switched the call site inState::fetch_public_account_details()to use it. The rest of the details path is unchanged.